handle_wrap: expose {un}refed check to JS #5834

Fishrock123 · 2016-03-21T20:06:26Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?

Affected core subsystem(s)

handle_wrap

Description of change

Give tools a consistent way to check if a handle is unrefed that works for all of our APIs that expose handles. E.g. would be very helpful to see which handles are umrefed in @AndreasMadsen's https://github.com/AndreasMadsen/dprof

(Some APIs, like timers, do not normally expose handles and are not addressed at that level here)

Refs: #5828
Refs: #5827

R= @trevnorris
cc @thlorenz

Fishrock123 · 2016-03-21T20:09:08Z

src/handle_wrap.cc

@@ -69,6 +81,7 @@ HandleWrap::HandleWrap(Environment* env,
  HandleScope scope(env->isolate());
  Wrap(object, this);
  env->handle_wrap_queue()->PushBack(this);
+  env->SetMethod(object, "isRefed", HandleWrap::IsRefed);


idk if it's better to have this be isUnrefed?

It probably should be isUnrefed(), but isRefed() seems a lot simpler to think about.

I think isRefed() sounds better ("is this holding the event loop open?").

This is not the way it's done. This has to be declared in every child class: https://github.com/nodejs/node/blob/v5.9.0/src/tcp_wrap.cc#L87

Otherwise you're setting an object property on an already constructed object.

oh that kinda sucks, alright.

Fishrock123 · 2016-03-21T20:09:18Z

(does this needs tests..?)

Fishrock123 · 2016-03-21T20:44:48Z

src/handle_wrap.cc

+    bool refed = (wrap->flags_ & kUnref) == 0;
+    args.GetReturnValue().Set(Boolean::New(env->isolate(), refed));
+  }
+}


Also: is it ok to return nothing of the handle is not alive?

returning nothing == returning undefined. but the JS signature returns bool. The question this is trying to answer is "Can this handle hold open the event loop?" So if !IsAlive() means should return false.

Noted, thanks.

So basically this can be boiled down to:

bool refed = IsAlive(wrap) && (wrap->flags_ & kUnref) == 0; args.GetReturnValue().Set(Boolean::New(env->isolate(), refed);

thlorenz · 2016-03-21T21:21:48Z

Yes, I think this needs tests. Ideally you'd pass tests similar to the ones in the PRs you referenced.
At that point we can also safely let this PR supersede those.

mscdex · 2016-03-21T21:27:15Z

Yes, tests are needed.

Fishrock123 · 2016-03-21T21:29:42Z

Does anyone have advice on how I can make a more generic test for this rather than testing everywhere that we expose a handle?

thlorenz · 2016-03-21T21:35:55Z

@Fishrock123 I guess you could test on C++ level (do we have C++ level tests?) and just create a handle and interact with it directly.
However I also believe that you should test this from a bit higher level for each case as well, at least for timers, sockets, pipes, etc.

jasnell · 2016-03-21T22:03:51Z

Does it have to be a method? Is it possible to have it as a read-only property? .refed

A quick test case would be worthwhile also.

Fishrock123 · 2016-03-21T22:18:39Z

@jasnell I guess this makes it explicit that you cannot just set it, but I could possibly change it otherwise.

jasnell · 2016-03-21T22:22:41Z

@Fishrock123 ... that makes sense. The additional () isn't too difficult to type ;-). LGTM with test cases added.

trevnorris · 2016-03-21T23:07:27Z

The implementation is incomplete. As noted in #5834 (comment) the method needs to be placed on the FunctionTemplate of the child class. This is tedious, but haven't found a better way to automate propagation of these JS methods to the C++ child classes. @bnoordhuis had an idea a while back, but can't remember what it was. Ben, you recall anything about this?

The test is simple enough:

const assert = require('assert');
const net = require('net');

const server = net.createServer(() => {}).listen(common.PORT);
server.unref();
assert.equal(false, server._handle.isRefed());

Remember this won't cover timer cases. To make the API parallel may need to implement a JS version that can be called on the JS timer handle instances.

AndreasMadsen · 2016-03-22T12:26:03Z

@Fishrock123 thanks for doing this. It would also be useful for just checking what keeps a process alive at any given time.

Fishrock123 · 2016-03-22T16:23:05Z

Updated with a test, some things:

I realized that some _handles we expose directly extend AsyncWrap instead of HandleWrap, and this currently does not cover those. To cover those that would mean moving the c++ check into AsyncWrap I think, and I'm not sure if that is possible/acceptable.

I'm not sure if it is possible to test spawn a net.Socket that uses a Pipe handle without making a child process, so I've just gone ahead and directly made a Pipe.

TTY's cannot be re-refed for some reason. The method does not exist in c++, and does not work if it is added.

Fishrock123 · 2016-03-22T16:23:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2025/

Fishrock123 · 2016-03-22T16:57:19Z

Hmm, getting this on Windows vs2015 and vcbt2015:

not ok 86 test-handle-wrap-isrefed.js
# events.js:155
# throw er; // Unhandled 'error' event
# ^
# 
# Error: read EISDIR
# at exports._errnoException (util.js:893:11)
# at TTY.onread (net.js:552:26)

Anyone know how to fix this test for that?

  const assert = makeAssert('isRefed() not working on tty_wrap');
  const ReadStream = require('tty').ReadStream;
  const tty = new ReadStream();
  assert(Object.getPrototypeOf(tty._handle).hasOwnProperty('isRefed'), true);
  assert(tty._handle.isRefed(), true);
  tty.unref();
  assert(tty._handle.isRefed(), false);

cjihrig · 2016-03-22T17:12:42Z

Do you need to pass a fd to the ReadStream() constructor?

Once the CI is happy, LGTM.

trevnorris · 2016-03-22T20:04:39Z

test/parallel/test-handle-wrap-isrefed.js

+  assert(timer._handle.isRefed(), false);
+  timer.ref();
+  assert(timer._handle.isRefed(), true);
+  timer.unref();


This depends on some non-obvious implementation details. If the timer is unref'd then the JS object will be moved to a TimerWrap that also isn't ref'd, correct?

See

node/lib/timers.js

Lines 460 to 485 in d3a7534

Timeout.prototype.unref = function() {

if (this._handle) {

this._handle.unref();

} else if (typeof this._onTimeout === 'function') {

var now = TimerWrap.now();

if (!this._idleStart) this._idleStart = now;

var delay = this._idleStart + this._idleTimeout - now;

if (delay < 0) delay = 0;

// Prevent running cb again when unref() is called during the same cb

if (this._called && !this._repeat) {

exports.unenroll(this);

return;

}

var handle = reuse(this);

this._handle = handle || new TimerWrap();

this._handle.owner = this;

this._handle[kOnTimeout] = unrefdHandle;

this._handle.start(delay, 0);

this._handle.domain = this.domain;

this._handle.unref();

}

return this;

};

timer.unref() assigns a ._handle for that specific timer, and is unlikely to change in the short term.

I realize that. What I failed to say is that a note of this should be placed in timer_wrap.cc where the method is added so future generations can immediately see why this non-obvious implementation detail actually works.

Where exactly do you mean? Isn't the same thing for the un(ref) methods on timer_wrap?

My point is it may be non-obvious why setTimeout(()=>{}, 100)._handle === undefined but setTimeout(()=>{}, 100).unref()._handle links to a Timer instance. So using isRefed() is conditional to the state of timer. So not having a uniform way to access that data from the public object instance could be confusing.

Sure, but the regular timers implementation will also benefit from this. e.g.

node/lib/timers.js

Lines 210 to 214 in cf94929

if (list._unrefed === true) {

delete unrefedLists[msecs];

} else {

delete refedLists[msecs];

}

So I'm not sure what I would say in handle_wrap

Fishrock123 · 2016-03-22T21:56:59Z

Ci with fd 0 for TTY: https://ci.nodejs.org/job/node-test-pull-request/2030/

Fishrock123 · 2016-03-22T22:28:35Z

@cjihrig still the same thing on those windows builds/platforms..

cjihrig · 2016-03-22T22:56:07Z

Crap. The only real use of tty.ReadStream I found was in src/node.js.

Fishrock123 · 2016-03-22T23:27:34Z

maybe cc @bnoordhuis

Fishrock123 · 2016-04-07T16:45:33Z

@nodejs/lts think we can get this in the next minor LTS release?

jasnell · 2016-04-07T17:24:42Z

Should be possible. It's just not yet clear when that next minor LTS release will be.

Fishrock123 · 2016-04-07T21:22:57Z

OK in hindsight this behavior regarding isAlive() is a huge mistake and this should not ship as is.

This needs to be accurate enough for timers to use too, and currently it isn't with either the following patches:

diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..3a57d86 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -133,8 +133,6 @@ function insert(item, unrefed) {
     list = new TimersList(msecs, unrefed);
     L.init(list);
     list._timer._list = list;
-
-    if (unrefed === true) list._timer.unref();
     list._timer.start(msecs, 0);

     lists[msecs] = list;
@@ -149,7 +147,7 @@ function TimersList(msecs, unrefed) {
   this._idleNext = null; // Create the list with the linkedlist properties to
   this._idlePrev = null; // prevent any unnecessary hidden class changes.
   this._timer = new TimerWrap();
-  this._unrefed = unrefed;
+  if (unrefed === true) this._timer.unref();
   this.msecs = msecs;
 }

@@ -207,7 +205,7 @@ function listOnTimeout() {
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
   this.close();
-  if (list._unrefed === true) {
+  if (list._timer.isRefed() === false) {
     delete unrefedLists[msecs];
   } else {
     delete refedLists[msecs];

diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..cdc0d1c 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -133,8 +133,6 @@ function insert(item, unrefed) {
     list = new TimersList(msecs, unrefed);
     L.init(list);
     list._timer._list = list;
-
-    if (unrefed === true) list._timer.unref();
     list._timer.start(msecs, 0);

     lists[msecs] = list;
@@ -149,7 +147,7 @@ function TimersList(msecs, unrefed) {
   this._idleNext = null; // Create the list with the linkedlist properties to
   this._idlePrev = null; // prevent any unnecessary hidden class changes.
   this._timer = new TimerWrap();
-  this._unrefed = unrefed;
+  if (unrefed === true) this._timer.unref();
   this.msecs = msecs;
 }

@@ -207,10 +205,10 @@ function listOnTimeout() {
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
   this.close();
-  if (list._unrefed === true) {
-    delete unrefedLists[msecs];
-  } else {
+  if (list._timer.isRefed() === true) {
     delete refedLists[msecs];
+  } else {
+    delete unrefedLists[msecs];
   }
 }

Fishrock123 · 2016-04-07T21:39:40Z

So... it does work if I close after the delete (duh), but the behavior is still pretty confusing..

Will probably remove the isAlive() check. (Which can be exposed separately if necessary)

MylesBorins · 2016-04-08T21:23:23Z

@Fishrock123 you may want to chime into the v6 thread before the rc is cut if you don't think this should land

This fixes my perceived usability issues with 7d8882b. Which, at the time of writing, has not landed in any release except v6 RCs. This should not be considered a breaking change due to that. It is useful if you have a handle, even if it has been closed, to be able to inspect whether that handle was unrefed or not. As such, this renames the method accordingly. If people need to check a handle's aliveness, that is a separate API we should consider exposing. Refs: nodejs#5834 PR-URL: nodejs#6204 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

This reverts commit 9bb5a5e. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

This reverts commit f938ef7. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

This reverts commit 7d8882b. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

Fishrock123 · 2016-05-17T14:36:55Z

For posterity, landing this requires this commit chain:

This allows third-party tools to check whether or not a handle that can be unreferenced is unreferenced at a particular time. Notably, this should be helpful for inspection via AsyncWrap. Also, this is useful even to node's internals, particularly timers. Refs: #5828 Refs: #5827 PR-URL: #5834 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

This fixes my perceived usability issues with 7d8882b. Which, at the time of writing, has not landed in any release except v6 RCs. This should not be considered a breaking change due to that. It is useful if you have a handle, even if it has been closed, to be able to inspect whether that handle was unrefed or not. As such, this renames the method accordingly. If people need to check a handle's aliveness, that is a separate API we should consider exposing. Refs: #5834 PR-URL: #6204 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 21, 2016

Fishrock123 reviewed Mar 21, 2016
View reviewed changes

thlorenz mentioned this pull request Mar 21, 2016

dgram: tracking unrefdness of dgram sockets #5828

Closed

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 21, 2016

Fishrock123 force-pushed the handle-unrefed branch from 502fe6d to a55ceff Compare March 22, 2016 16:18

trevnorris reviewed Mar 22, 2016
View reviewed changes

Fishrock123 force-pushed the handle-unrefed branch from a55ceff to 40c81d2 Compare March 22, 2016 21:52

Fishrock123 mentioned this pull request Mar 30, 2016

test: ensure existence of _handle property #5916

Closed

4 tasks

Fishrock123 force-pushed the handle-unrefed branch from 40c81d2 to abd0daf Compare March 30, 2016 17:02

Fishrock123 merged commit 7d8882b into nodejs:master Apr 7, 2016

Fishrock123 added dont-land-on-v5.x labels Apr 7, 2016

MylesBorins removed the lts-watch-v4.x label Apr 8, 2016

bnoordhuis mentioned this pull request Apr 11, 2016

src: fix -Wunused-variable compiler warning #6129

Merged

Fishrock123 mentioned this pull request Apr 14, 2016

handle_wrap: IsRefed -> Unrefed, no isAlive check #6204

Merged

3 tasks

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check"

635fb96

This reverts commit 9bb5a5e. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

Fishrock123 mentioned this pull request Apr 26, 2016

Revert handle wrap refed check #6401

Closed

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016

Revert "src: fix -Wunused-variable compiler warning"

6b025df

This reverts commit f938ef7. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016

Revert "handle_wrap: expose an isRefed() check to JS"

80a2229

This reverts commit 7d8882b. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

bnoordhuis added the dont-land-on-v6.x label Apr 27, 2016

Fishrock123 removed dont-land-on-v4.x labels May 17, 2016

Fishrock123 mentioned this pull request May 17, 2016

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6546

Merged

3 tasks

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

Fishrock123 mentioned this pull request May 19, 2016

Discuss backporting HandleWrap#hasRef() in a Minor nodejs/Release#108

Closed

	Timeout.prototype.unref = function() {
	if (this._handle) {
	this._handle.unref();
	} else if (typeof this._onTimeout === 'function') {
	var now = TimerWrap.now();
	if (!this._idleStart) this._idleStart = now;
	var delay = this._idleStart + this._idleTimeout - now;
	if (delay < 0) delay = 0;

	// Prevent running cb again when unref() is called during the same cb
	if (this._called && !this._repeat) {
	exports.unenroll(this);
	return;
	}

	var handle = reuse(this);

	this._handle = handle \|\| new TimerWrap();
	this._handle.owner = this;
	this._handle[kOnTimeout] = unrefdHandle;
	this._handle.start(delay, 0);
	this._handle.domain = this.domain;
	this._handle.unref();
	}
	return this;
	};

	if (list._unrefed === true) {
	delete unrefedLists[msecs];
	} else {
	delete refedLists[msecs];
	}

handle_wrap: expose {un}refed check to JS #5834

handle_wrap: expose {un}refed check to JS #5834

Conversation

Fishrock123 commented Mar 21, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thlorenz commented Mar 21, 2016

mscdex commented Mar 21, 2016

Fishrock123 commented Mar 21, 2016

thlorenz commented Mar 21, 2016

jasnell commented Mar 21, 2016

Fishrock123 commented Mar 21, 2016

jasnell commented Mar 21, 2016

trevnorris commented Mar 21, 2016

AndreasMadsen commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

cjihrig commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

cjihrig commented Mar 22, 2016

Fishrock123 commented Mar 22, 2016

Fishrock123 commented Apr 7, 2016

jasnell commented Apr 7, 2016

Fishrock123 commented Apr 7, 2016

Fishrock123 commented Apr 7, 2016

MylesBorins commented Apr 8, 2016

Fishrock123 commented May 17, 2016 • edited Loading

Fishrock123 commented May 17, 2016 •

edited

Loading